fix: only this session's own worktree counts as its worktree - #15
Conversation
`locate` took `recorded_branch` but never passed it to `inspect`, so any worktree of the same repository at the recorded path counted as the session's own. Remove a session's worktree, create another at that path on a different branch, resume the session — and the agent commits its work to whatever branch it found. In the feature whose entire purpose is isolation. inspect now requires the checked-out branch to match, alongside the shared git directory and the worktree top level. A detached HEAD deliberately does not match: the recording names a branch, and resuming onto a detached head would leave the work unreachable by that name. Also: a regular file at the recorded path was reported Gone, which sends resume down the recreate path where `git worktree add` fails on the occupied path with an error that never mentions the file. Anything that exists but is not the session's worktree is now Foreign, so resume reports Occupied and names what is in the way. Found by CodeRabbit on #14 and left unresolved when that PR merged. Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
📝 WalkthroughWalkthroughWorktree inspection validates repository identity, top-level path, branch, and path type. Detached, differently branched, foreign, file, and dangling symlink paths are treated as occupied. Integration tests cover each case. ChangesWorktree validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/src/worktree.rs`:
- Around line 272-276: Update the path classification logic around the
exists/is_dir checks to use symlink_metadata() instead of Path::exists(),
returning Candidate::Gone only when metadata reports the path is genuinely
missing and Candidate::Foreign for dangling symlinks or other metadata errors.
Preserve the existing directory classification behavior for resolvable paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a81ef1e7-c619-4a81-b25c-1e6deab6cb7c
📒 Files selected for processing (1)
crates/cli/src/worktree.rs
|
Worked the same finding in parallel (my commits are on
So an agent that runs The version I pushed treats detached HEAD as agreement and only a different named branch as proof of a different worktree: /// Whether the worktree at `path` still answers for `branch`. A detached
/// HEAD counts: an interrupted rebase or a `git checkout <sha>` the agent
/// ran is the work this module exists to keep, and refusing to resume into
/// it would strand exactly that. Only a different *branch* proves a
/// different worktree.
fn head_agrees(path: &Path, branch: &str) -> bool { ... }The residue that leaves — a stranger worktree at the recorded path that also happens to be detached — needs someone to delete the session worktree, prune, and add a new detached one at that exact UUID path. Cheap to give up; a mid-rebase resume is not. One smaller thing: The rest of your version is better than mine: the separate focused tests read more clearly than my extended one, and the empty-output guard in Not touching this branch — flagging it for you to decide. |
Two more ways the recorded path can hold something that is not this session's worktree. `Path::exists` follows symlinks, so a dangling link at the recorded path reported itself absent. Resume then took the recreate branch and git refused, because the link does occupy the path. `symlink_metadata` sees the link itself. A detached worktree has no branch to agree with, and accepting one would let a resumed session commit where the recorded branch name can never reach the work again. Worth stating because the obvious shape gets it backwards: `git symbolic-ref -q HEAD` exits nonzero when detached, so treating command failure as agreement silently accepts exactly the case it should refuse. Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
Steel-tech
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/worktree.rs (1)
275-279: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject symlink paths in
inspect.
path.is_dir()follows symlinks. A symlink target can pass all repository, top-level, and branch checks, causinglocateto returnLocation::Usefor the target checkout. Usesymlink_metadataand requiremetadata.is_dir(). Add a regression test with a symlink to the anchor and a matching recorded branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cli/src/worktree.rs` around lines 275 - 279, Update inspect’s path validation around symlink_metadata so it rejects symlink paths by requiring the returned metadata to be a directory, rather than using path.is_dir(). Add a regression test covering a symlink to the anchor with a matching recorded branch, ensuring locate does not return Location::Use for the target checkout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cli/src/worktree.rs`:
- Around line 275-279: Update inspect’s path validation around symlink_metadata
so it rejects symlink paths by requiring the returned metadata to be a
directory, rather than using path.is_dir(). Add a regression test covering a
symlink to the anchor with a matching recorded branch, ensuring locate does not
return Location::Use for the target checkout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed1e3f66-a713-474a-8a05-6c7e82b3e263
📒 Files selected for processing (1)
crates/cli/src/worktree.rs
1 participant
Summary
#14shipped--bg --worktreewith a review finding left unresolved, plus two more the same check was hiding. All three are the same bug class: something at the recorded path that is not this session's worktree being treated as if it were.In the feature whose entire purpose is isolation, that means a resumed agent commits its work somewhere it doesn't belong.
The three cases
locateacceptedrecorded_branchbut never passed it toinspect, andinspectasked only whether the path was a directory belonging to the same repository.Live→ resume commits to that branchOccupiedGone→ recreate, thengit worktree addfails without ever naming the fileOccupiedGone(Path::existsfollows links and says absent)OccupiedLiveOccupiedThe detached case is worth calling out, because the obvious implementation gets it backwards.
git symbolic-ref -q HEADexits nonzero when detached, so a check shaped!status.success() || name == branchreads command failure as agreement and accepts exactly the case it should refuse. Verified against real git rather than reasoned about. Resuming onto a detached head would leave the work unreachable by the branch name the session recorded.Verification
Four git-backed tests, and every one was mutation-tested — the fix reverted, the test confirmed failing, the fix restored:
That step is not ceremony here. This repo has already shipped a regression guard that passed with its bug still present, which is how the branch-identity bug reached
mainin the first place.Workspace: 131 tests pass, clippy clean under
-D warnings, fmt clean.One test detail: git keeps its administrative entry for a deleted worktree, so a second
worktree addat the same path is refused with "missing but already registered". The tests callgit worktree prunefirst — which is also what a real user clearing a directory would hit.Post-Deploy Monitoring & Validation
No runtime change beyond the corrected classification. The observable difference: resuming a session whose recorded path now holds something else fails with
Occupied, naming the path and expected branch, instead of silently running there. A report of resume newly refusing where it used to proceed is this fix working — check whether the path really holds that session's own branch.Refs #13
Summary by CodeRabbit